Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#1560] Feature/intermediate representation data format #1561

Merged
merged 8 commits into from Nov 9, 2020
Merged

[#1560] Feature/intermediate representation data format #1561

merged 8 commits into from Nov 9, 2020

Conversation

zambrovski
Copy link
Contributor

This pull request fixes #1560 and provides the ability to ask the IntermediateEventRepresentation for the Serializer and if its data can be converted to a target format.

This is useful if you want to detect if the Upcaster can upcast an event which is serialized in a certain way.

See https://discuss.axoniq.io/t/upcasters-for-json-and-xml/2799 for more details.

@smcvb smcvb added Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: In Progress Use to signal this issue is actively worked on. Type: Enhancement Use to signal an issue enhances an already existing feature of the project. labels Oct 21, 2020
@smcvb smcvb added this to the Release 4.5 milestone Oct 21, 2020
smcvb
smcvb requested changes Oct 21, 2020
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of small pointers, although in general I think this is good.

...in/java/org/axonframework/serialization/upcasting/event/IntermediateEventRepresentation.java Outdated Show resolved Hide resolved
...in/java/org/axonframework/serialization/upcasting/event/IntermediateEventRepresentation.java Outdated Show resolved Hide resolved
...in/java/org/axonframework/serialization/upcasting/event/IntermediateEventRepresentation.java Outdated Show resolved Hide resolved
...est/java/org/axonframework/serialization/upcasting/event/IntermediateRepresentationTest.java Outdated Show resolved Hide resolved
...est/java/org/axonframework/serialization/upcasting/event/IntermediateRepresentationTest.java Outdated Show resolved Hide resolved
...est/java/org/axonframework/serialization/upcasting/event/IntermediateRepresentationTest.java Outdated Show resolved Hide resolved
zambrovski and others added 3 commits October 21, 2020 14:27
@zambrovski @smcvb
Update messaging/src/main/java/org/axonframework/serialization/upcast…
69ced8e 
…ing/event/IntermediateEventRepresentation.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@zambrovski @smcvb
Update messaging/src/main/java/org/axonframework/serialization/upcast…
1acaf17 
…ing/event/IntermediateEventRepresentation.java

Co-authored-by: Steven van Beelen <steven.vanbeelen@axoniq.io>
@zambrovski
chore: work in comments of @smbcvb
204a1b3
@zambrovski zambrovski requested a review from smcvb October 21, 2020 12:34
smcvb
smcvb approved these changes Oct 21, 2020
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

abuijze
abuijze previously requested changes Oct 21, 2020
View reviewed changes
Copy link
Member

@abuijze abuijze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor change request.
Also, we need to decide whether it's a good idea to expose the Serializer. After our discussion on discuss.axoniq.io, I am not too sure we should do it :)

...rc/main/java/org/axonframework/serialization/upcasting/event/InitialEventRepresentation.java Outdated Show resolved Hide resolved
...rc/main/java/org/axonframework/serialization/upcasting/event/InitialEventRepresentation.java Outdated Show resolved Hide resolved
...in/java/org/axonframework/serialization/upcasting/event/IntermediateEventRepresentation.java Outdated Show resolved Hide resolved
@zambrovski
Copy link
Contributor Author

@abuijze you are probably right, there is no reason to have it inside the upcaster. And if you need it, you can access it via Injection... So I removed the access method and the test...

smcvb
smcvb requested changes Oct 22, 2020
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New concern around the test class. Otherwise I think we're going the right way.

...in/java/org/axonframework/serialization/upcasting/event/IntermediateEventRepresentation.java Outdated Show resolved Hide resolved
...est/java/org/axonframework/serialization/upcasting/event/IntermediateRepresentationTest.java Outdated Show resolved Hide resolved
smcvb
smcvb approved these changes Oct 23, 2020
View reviewed changes
Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concerns have been addressed, hence approving.

@smcvb smcvb requested a review from abuijze October 23, 2020 07:27
@smcvb smcvb changed the title Feature/intermediate representation data format [#1560] Feature/intermediate representation data format Oct 23, 2020
lfgcampos
lfgcampos approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@lfgcampos lfgcampos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smcvb smcvb dismissed abuijze’s stale review November 9, 2020 12:01

Concerns have been addressed.

@smcvb smcvb merged commit 11429f9 into AxonFramework:master Nov 9, 2020
@smcvb smcvb removed the Status: In Progress Use to signal this issue is actively worked on. label Nov 9, 2020
@smcvb smcvb added the Status: Resolved Use to signal that work on this issue is done. label Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smcvb smcvb smcvb approved these changes

@lfgcampos lfgcampos lfgcampos approved these changes

@m1l4n54v1c m1l4n54v1c Awaiting requested review from m1l4n54v1c

@abuijze abuijze Awaiting requested review from abuijze

Assignees

@zambrovski zambrovski

Labels
Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: Resolved Use to signal that work on this issue is done. Type: Enhancement Use to signal an issue enhances an already existing feature of the project.
Projects
None yet
Milestone
Release 4.5
4 participants
@zambrovski @abuijze @smcvb @lfgcampos